Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DEVHAS-380] Have the Application controller add/remove components from its model #363

Merged
merged 10 commits into from
Aug 1, 2023
Merged

[DEVHAS-380] Have the Application controller add/remove components from its model #363

merged 10 commits into from
Aug 1, 2023

Conversation

johnmcollier
Copy link
Member

What does this PR do?:

This PR updates the logic that handles the Application's devfile model by moving the code in the Component controller that adds/removes components from it, and moves it to the Application controller.

With these changes:

  • Application reconcile is triggered by a Component creation or deletion
  • During each reconcile of an Application, the Application controller retrieves the list of Components that set the Application as its owner, and updates its devfile model accordingly

Which issue(s)/story(ies) does this PR fixes:

https://issues.redhat.com/browse/DEVHAS-380

PR acceptance criteria:

  • Unit/Functional tests

  • Documentation

  • Client Impact

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot requested review from elsony and yangcao77 July 12, 2023 14:58
@johnmcollier johnmcollier changed the title [WIP] [DEVHAS-380] Have the Application controller add/remove components from its model [DEVHAS-380] Have the Application controller add/remove components from its model Jul 18, 2023
@johnmcollier
Cleanup
b288f9f 
Signed-off-by: John Collier <[email protected]>
@johnmcollier
Fix tests
595cb45 
Signed-off-by: John Collier <[email protected]>
@johnmcollier
Cleanup and fix test error
e9103be 
Signed-off-by: John Collier <[email protected]>
@johnmcollier
Go fmt
fe05891 
Signed-off-by: John Collier <[email protected]>
maysunfaisal
maysunfaisal reviewed Jul 21, 2023
View reviewed changes
controllers/application_controller.go
Comment on lines +217 to +218
devWorkspacesSpec.Attributes = nil
devWorkspacesSpec.Projects = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait why do we do this again

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make comparisons of the added git projects (under projects in the devfile spec) and container image projects (under attributes) easier in the devfile app model. Basically we create a copy of the devworkspace spec, nil out the attributes and projects, add the components to it, and compare with the existing devworkspace spec.

maysunfaisal
maysunfaisal reviewed Jul 21, 2023
View reviewed changes
controllers/mapper.go Outdated Show resolved Hide resolved
@maysunfaisal
Copy link
Member

/retest

@johnmcollier @maysunfaisal
Update controllers/mapper.go
1cb8d12 
Co-authored-by: Maysun Faisal <[email protected]>
@openshift-ci openshift-ci bot added the lgtm label Jul 24, 2023
@johnmcollier
Copy link
Member Author

Need to wait for pact tests to be updated before merging

/hold

@Katka92
Copy link
Contributor

Katka92 commented Aug 1, 2023

/retest

@openshift-ci openshift-ci bot removed the lgtm label Aug 1, 2023
@johnmcollier
Copy link
Member Author

/unhold

@codecov
Copy link

codecov bot commented Aug 1, 2023 •
edited
Loading

Codecov Report

Patch coverage: 83.23% and project coverage change: -0.52% ⚠️

Comparison is base (1afd7ae) 84.61% compared to head (1fca333) 84.09%.
Report is 77 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #363      +/-   ##
==========================================
- Coverage   84.61%   84.09%   -0.52%     
==========================================
  Files          27       31       +4     
  Lines        3530     4226     +696     
==========================================
+ Hits         2987     3554     +567     
- Misses        402      507     +105     
- Partials      141      165      +24
Files Changed Coverage Δ
pkg/devfile/errors.go 100.00% <ø> (ø)
controllers/component_controller.go 73.87% <62.31%> (-3.72%) ⬇️
cdq-analysis/pkg/devfile.go 68.30% <68.30%> (ø)
cdq-analysis/pkg/componentdetectionquery.go 69.28% <69.28%> (ø)
controllers/componentdetectionquery_controller.go 74.51% <71.42%> (+3.97%) ⬆️
controllers/component_controller_finalizer.go 67.34% <75.00%> (-7.66%) ⬇️
pkg/github/token_mock.go 75.67% <75.67%> (-24.33%) ⬇️
controllers/application_controller.go 80.00% <76.81%> (-1.53%) ⬇️
cdq-analysis/pkg/detect.go 73.38% <79.54%> (ø)
pkg/github/token.go 87.74% <84.95%> (-7.65%) ⬇️
... and 17 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@openshift-ci openshift-ci bot added the lgtm label Aug 1, 2023
@openshift-ci
Copy link

openshift-ci bot commented Aug 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnmcollier, maysunfaisal, yangcao77

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [johnmcollier,maysunfaisal,yangcao77]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@johnmcollier johnmcollier merged commit 692c54f into redhat-appstudio:main Aug 1, 2023
5 of 7 checks passed
@maysunfaisal maysunfaisal mentioned this pull request Aug 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yangcao77 yangcao77 yangcao77 approved these changes

@maysunfaisal maysunfaisal maysunfaisal approved these changes

@elsony elsony Awaiting requested review from elsony

Assignees

@yangcao77 yangcao77

@maysunfaisal maysunfaisal

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johnmcollier @maysunfaisal @Katka92 @yangcao77